bpo-30899: IDLE: Add idle config parser unittest #2662

mlouielu · 2017-07-11T06:13:38Z

No description provided.

mention-bot · 2017-07-11T06:13:39Z

@mlouielu, thanks for your PR! By analyzing the history of the files in this pull request, we identified @terryjreedy to be a potential reviewer.

terryjreedy

Without looking at the classes, looking good.

terryjreedy · 2017-07-11T06:56:44Z

Lib/idlelib/idle_test/test_config.py

+        self.assertEqual(parser.GetOptionList('not exist'), [])
+
+    def test_load_file(self):
+        config_path = support.findfile('cfgparser.1')


Is this a file in test dir?

Yes, borrow from test_configparser.py

terryjreedy · 2017-07-11T06:58:36Z

Lib/idlelib/idle_test/test_config.py

@@ -3,6 +3,9 @@
 Much is tested by opening config dialog live or in test_configdialog.
 Coverage: 27%
 '''
+import os
+import tempfile
+from test import support
 from test.support import captured_stderr


One style or other, not both. Either add findfile to second or 'support.' to 'captured_stderr' wherever it is. EDIT: fixed.

terryjreedy · 2017-07-11T06:59:17Z

Lib/idlelib/idle_test/test_config.py

+        one = a string
+        two = true
+        three = false
+    """


Slightly redundant, but OK as is.

terryjreedy · 2017-07-11T07:04:33Z

Lib/idlelib/idle_test/test_config.py

+        self.assertEqual(parser.Get('two', 'two'), 'true')
+        self.assertEqual(parser.Get('two', 'three'), 'false')
+        self.assertEqual(parser.Get('not', 'exist'), None)
+        self.assertEqual(parser.Get('not', 'exist', default='DEFAULT'), 'DEFAULT')


This could be done with a subtest with 5 cases and a subtest with 4 cases and the remaining test. But I think it not worthwhile here. What I have done in situations like this and would like to do here is to name the bound method, "equal = self.assertEqual", and then use that: "equal(parser.Get(...., False)". It is then easier to focus on what is being compared.
EDIT: fixed (eq is fine).

serhiy-storchaka · 2017-07-11T08:03:56Z

Lib/idlelib/idle_test/test_config.py

+        eq = self.assertEqual
+
+        # Test with type
+        eq(parser.Get('one', 'one', type='bool'), False)


It is better to use assertIs(). Since False and True are singletons, this tests both type and value. Otherwise parser.Get() returning 0 will pass the test.

Let us see whether configparser passes the stricter test.

serhiy-storchaka · 2017-07-11T08:05:49Z

Lib/idlelib/idle_test/test_config.py

+        eq(parser.Get('two', 'three'), 'false')
+
+        # If option not exist, should return None, or default
+        eq(parser.Get('not', 'exist'), None)


assertIsNone()

Agreed here and below.

serhiy-storchaka · 2017-07-11T08:06:44Z

Lib/idlelib/idle_test/test_config.py

+
+        parser.AddSection('Foo')
+        parser.AddSection('Bar')
+        self.assertEqual(parser.IsEmpty(), True)


assertTrue().

Or assertIs() if you want to check that exact bool is returned. But I think this is redundant.

serhiy-storchaka · 2017-07-11T08:07:08Z

Lib/idlelib/idle_test/test_config.py

+        parser.AddSection('Foo')
+        parser.AddSection('Bar')
+        parser.SetOption('Foo', 'bar', 'false')
+        self.assertEqual(parser.IsEmpty(), False)


assertFalse().

serhiy-storchaka · 2017-07-11T08:08:04Z

Lib/idlelib/idle_test/test_config.py

+
+        # Set option to not exist section should create section and return True
+        self.assertEqual(parser.SetOption('Bar', 'bar', 'true'), True)
+        self.assertEqual(sorted(parser.sections()), ['Bar', 'Foo'])


You can use assertCountEqual().

Seems like a looser test, but I read the doc, as it is new to me.

I think using assertCountEqual() is OK, ConfigParser were using assertEqual to test, we only need to care about the item is correct or not here.

serhiy-storchaka · 2017-07-11T08:08:36Z

Lib/idlelib/idle_test/test_config.py

+        parser.AddSection('Foo')
+        parser.SetOption('Foo', 'bar', 'true')
+
+        self.assertEqual(parser.RemoveOption('Foo', 'bar'), True)


assertTrue()/assertFalse()

terryjreedy · 2017-07-11T09:02:29Z

Serhiy, thanks for the review. I still have to remember to use the specific tests. I think the code reads better.

terryjreedy · 2017-07-12T03:02:57Z

Louie, I think this is ready to merge and plan to do so tomorrow. Review changes if you can. I re-ordered the functions in one of the classes and corresponding test class so the order makes sense and is the same in both. So the actual change is less drastic than it might look at first. If I had thought about it, I might have done the re-order in a separate commit. I added a few test to complete coverage.

mlouielu · 2017-07-12T03:11:44Z

@terryjreedy I take a brief look and seems good. I'll take the deep review today.

mlouielu · 2017-07-12T10:29:27Z

@terryjreedy Fix some comment and the cfg_type_changed with unittest assert. Otherwise looks good.

csabella · 2017-07-12T11:14:53Z

Lib/idlelib/idle_test/test_config.py

@@ -26,6 +30,161 @@ def tearDownModule():
    idleConf.userCfg = usercfg


+class IdleConfParserTest(unittest.TestCase):
+    """Test ththat IdleConfParser works"""


Comment fix.

terryjreedy · 2017-07-12T17:51:23Z

Louie, you need to follow the same comment rule you have reminded Cheryl of.
# Capitalize first word, end with period.
It is great to have these two classes completely covered.

…arser subclasses. (pythonGH-2662) Patch by Louie Lu. (cherry picked from commit 50c9435)

…arser subclasses. (GH-2662) (#2685) Patch by Louie Lu. (cherry picked from commit 50c9435)

mlouielu · 2017-07-13T02:19:56Z

Thanks @terryjreedy @csabella @serhiy-storchaka for reviewing this, learn a lot.

the-knights-who-say-ni added the CLA signed label Jul 11, 2017

bpo-30899: IDLE: Add idle config parser unittest

9bdacc7

mlouielu changed the title ~~bpo-30899: Add idle config parser unittest~~ bpo-30899: IDLE: Add idle config parser unittest Jul 11, 2017

mlouielu force-pushed the bpo-30899 branch from 6d0878f to 9bdacc7 Compare July 11, 2017 06:23

News blurb

65ef731

terryjreedy requested changes Jul 11, 2017

View reviewed changes

mlouielu added 2 commits July 11, 2017 15:20

Addressed Terry's requests

2527171

Merge branch 'bpo-30899' of github.com:mlouielu/cpython into bpo-30899

75fac8e

serhiy-storchaka reviewed Jul 11, 2017

View reviewed changes

Addressed Serhiy's comments

5035a20

terryjreedy approved these changes Jul 11, 2017

View reviewed changes

Reorder some functions, add tests, fix nitpicks.

019402c

mlouielu added 3 commits July 12, 2017 17:53

Add some comments

2a7e478

Fix assert always be true

d519d34

Fix save_all return bad result

4fa460f

csabella reviewed Jul 12, 2017

View reviewed changes

Update test_config.py

6747e7c

terryjreedy merged commit 50c9435 into python:master Jul 12, 2017

terryjreedy pushed a commit to terryjreedy/cpython that referenced this pull request Jul 12, 2017

[3.6] bpo-30899: Add unittests, 100% coverage, for IDLE's two ConfigP…

7b8954a

…arser subclasses. (pythonGH-2662) Patch by Louie Lu. (cherry picked from commit 50c9435)

terryjreedy mentioned this pull request Jul 12, 2017

[3.6] bpo-30899: Add unittests, 100% coverage, for IDLE's two ConfigP… #2685

Merged

terryjreedy added a commit that referenced this pull request Jul 12, 2017

[3.6] bpo-30899: Add unittests, 100% coverage, for IDLE's two ConfigP…

a9bf62c

…arser subclasses. (GH-2662) (#2685) Patch by Louie Lu. (cherry picked from commit 50c9435)

Uh oh!

bpo-30899: IDLE: Add idle config parser unittest #2662

bpo-30899: IDLE: Add idle config parser unittest #2662

Uh oh!

Conversation

mlouielu commented Jul 11, 2017

Uh oh!

mention-bot commented Jul 11, 2017

Uh oh!

terryjreedy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

terryjreedy Jul 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

terryjreedy Jul 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

terryjreedy Jul 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

terryjreedy Jul 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

terryjreedy commented Jul 11, 2017

Uh oh!

terryjreedy commented Jul 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mlouielu commented Jul 12, 2017

Uh oh!

mlouielu commented Jul 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

terryjreedy commented Jul 12, 2017

Uh oh!

mlouielu commented Jul 13, 2017

Uh oh!

Uh oh!

terryjreedy Jul 11, 2017 •

edited

Loading

terryjreedy Jul 11, 2017 •

edited

Loading

terryjreedy Jul 11, 2017 •

edited

Loading

terryjreedy Jul 11, 2017 •

edited

Loading

terryjreedy commented Jul 12, 2017 •

edited

Loading